Skip to content

Fixing regex bypass issue #4336

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Fixing regex bypass issue #4336

wants to merge 4 commits into from
+399 −312

Conversation

zane-neo
Copy link
Collaborator

Description

Fix connector regex bypass issue

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@zane-neo zane-neo had a problem deploying to ml-commons-cicd-env October 22, 2025 03:27 — with GitHub Actions Failure
@zane-neo zane-neo had a problem deploying to ml-commons-cicd-env October 22, 2025 03:27 — with GitHub Actions Failure
@zane-neo
fix failure ITs
6289562 
Signed-off-by: zane-neo <[email protected]>
@zane-neo zane-neo temporarily deployed to ml-commons-cicd-env October 22, 2025 04:56 — with GitHub Actions Inactive
@zane-neo zane-neo had a problem deploying to ml-commons-cicd-env October 22, 2025 04:56 — with GitHub Actions Failure
@zane-neo zane-neo temporarily deployed to ml-commons-cicd-env October 22, 2025 04:56 — with GitHub Actions Inactive
@zane-neo zane-neo temporarily deployed to ml-commons-cicd-env October 22, 2025 06:50 — with GitHub Actions Inactive
@zane-neo zane-neo temporarily deployed to ml-commons-cicd-env October 22, 2025 06:50 — with GitHub Actions Inactive
@codecov
Copy link

codecov bot commented Oct 22, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 77.63158% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.12%. Comparing base (66bcde7) to head (6289562).

Files with missing lines Patch % Lines
...rch/ml/common/httpclient/ValidatingHttpClient.java 65.11% 11 Missing and 4 partials ⚠️
...engine/algorithms/remote/AwsConnectorExecutor.java 91.66% 1 Missing ⚠️
...e/algorithms/remote/HttpJsonConnectorExecutor.java 90.90% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##               main    #4336      +/-   ##
============================================
- Coverage     80.14%   80.12%   -0.03%     
- Complexity    10172    10174       +2     
============================================
  Files           854      855       +1     
  Lines         44219    44239      +20     
  Branches       5113     5113              
============================================
+ Hits          35440    35447       +7     
- Misses         6630     6641      +11     
- Partials       2149     2151       +2
Flag Coverage Δ
ml-commons 80.12% <77.63%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@zane-neo zane-neo had a problem deploying to ml-commons-cicd-env October 22, 2025 07:55 — with GitHub Actions Failure
Hailong-am
Hailong-am reviewed Oct 22, 2025
View reviewed changes
common/src/main/java/org/opensearch/ml/common/httpclient/MLHttpClientFactory.java Outdated
Duration connectionTimeout,
Duration readTimeout,
int maxConnections,
AtomicBoolean connectorPrivateIpEnabled
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use bool here? what's the reason to use atomicBoolean

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree that boolean is better, I tried to change all the occurrence of this connectorPrivateIpEnabled to boolean all the way back to where it's defined, but there's more UTs need to be changed. But changing to boolean is correct direction, let me try again this change.

@peterzhuamazon peterzhuamazon mentioned this pull request Oct 22, 2025
Closed
65 tasks
ylwu-amzn
ylwu-amzn reviewed Oct 22, 2025
View reviewed changes
common/src/main/java/org/opensearch/ml/common/httpclient/ValidatingHttpClient.java Outdated
import software.amazon.awssdk.http.async.SdkAsyncHttpClient;

@Log4j2
public class ValidatingHttpClient implements SdkAsyncHttpClient {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest use a better class name.

rithin-pullela-aws
rithin-pullela-aws approved these changes Oct 22, 2025
View reviewed changes
Copy link
Contributor

@rithin-pullela-aws rithin-pullela-aws left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the comment from @ylwu-amzn, looks good to me!

@ylwu-amzn
Copy link
Collaborator

let's backport to 3.3

@peterzhuamazon peterzhuamazon mentioned this pull request Oct 23, 2025
Open
65 tasks
@zane-neo zane-neo had a problem deploying to ml-commons-cicd-env October 23, 2025 02:53 — with GitHub Actions Failure
@zane-neo zane-neo had a problem deploying to ml-commons-cicd-env October 23, 2025 02:53 — with GitHub Actions Failure
@zane-neo zane-neo had a problem deploying to ml-commons-cicd-env October 23, 2025 03:01 — with GitHub Actions Failure
@zane-neo zane-neo had a problem deploying to ml-commons-cicd-env October 23, 2025 03:01 — with GitHub Actions Failure
@zane-neo zane-neo had a problem deploying to ml-commons-cicd-env October 23, 2025 03:23 — with GitHub Actions Failure
@zane-neo zane-neo had a problem deploying to ml-commons-cicd-env October 23, 2025 03:23 — with GitHub Actions Failure
@zane-neo zane-neo had a problem deploying to ml-commons-cicd-env October 23, 2025 03:52 — with GitHub Actions Failure
@zane-neo zane-neo temporarily deployed to ml-commons-cicd-env October 23, 2025 03:52 — with GitHub Actions Inactive
@zane-neo zane-neo deployed to ml-commons-cicd-env October 23, 2025 03:52 — with GitHub Actions Active
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ylwu-amzn ylwu-amzn ylwu-amzn left review comments

@xinyual xinyual xinyual approved these changes

@b4sjoo b4sjoo Awaiting requested review from b4sjoo b4sjoo is a code owner

@dhrubo-os dhrubo-os Awaiting requested review from dhrubo-os dhrubo-os is a code owner

@mingshl mingshl Awaiting requested review from mingshl mingshl is a code owner

@jngz-es jngz-es Awaiting requested review from jngz-es jngz-es is a code owner

@model-collapse model-collapse Awaiting requested review from model-collapse model-collapse is a code owner

@rbhavna rbhavna Awaiting requested review from rbhavna rbhavna is a code owner

@Zhangxunmt Zhangxunmt Awaiting requested review from Zhangxunmt Zhangxunmt is a code owner

@austintlee austintlee Awaiting requested review from austintlee austintlee is a code owner

@HenryL27 HenryL27 Awaiting requested review from HenryL27 HenryL27 is a code owner

@sam-herman sam-herman Awaiting requested review from sam-herman sam-herman is a code owner

@pyek-bot pyek-bot Awaiting requested review from pyek-bot pyek-bot is a code owner

+2 more reviewers

@Hailong-am Hailong-am Hailong-am left review comments

@rithin-pullela-aws rithin-pullela-aws rithin-pullela-aws approved these changes

Reviewers whose approvals may not affect merge requirements

At least 2 approving reviews are required to merge this pull request.

Assignees

No one assigned

Labels

backport 3.3

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@zane-neo @ylwu-amzn @xinyual @Hailong-am @rithin-pullela-aws